Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[native] Disable plan validation failure for timestamp with timezone and ipaddress types for native execution. #24040

Merged
merged 1 commit into from
Nov 25, 2024
Merged

[native] Disable plan validation failure for timestamp with timezone and ipaddress types for native execution. #24040

merged 1 commit into from
Nov 25, 2024

Conversation

amitkdutta
Copy link
Contributor

@amitkdutta amitkdutta commented Nov 14, 2024 •
edited
Loading

Description

As native engine now supports timestamp with timezone and ipaddress types, no need to make queries fail with plan validation by default. This change disables such validation failures by default.

Motivation and Context

Keeping timestamp and ipaddress by default enabled like java engine.

Impact

No new impact. If a production system explicitly set these configs, now they can remove those as default is enabled.

Test Plan

Updated existing tests.

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Nov 14, 2024
@amitkdutta amitkdutta marked this pull request as draft November 14, 2024 01:28
@amitkdutta amitkdutta changed the title [native] Disable plan validation failure for timestamp with timezone … [native] Disable plan validation failure for timestamp with timezone and ipaddress types for native execution. Nov 14, 2024
@amitkdutta amitkdutta marked this pull request as ready for review November 14, 2024 04:49
@tdcmeehan
Copy link
Contributor

Should we also delete the code CheckUnsupportedPrestissimoTypes?

feilong-liu
feilong-liu previously approved these changes Nov 14, 2024
View reviewed changes
@aditi-pandit
Copy link
Contributor

Should we also delete the code CheckUnsupportedPrestissimoTypes?

Agree. We should remove the code as well. It doesn't provide any value anymore. We can re-add it if we want to bring the config back.

pramodsatya
pramodsatya reviewed Nov 19, 2024
View reviewed changes
Copy link
Contributor

@pramodsatya pramodsatya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amitkdutta! The failing e2e tests with IpAddress and TimestampWithTimeZone types in presto-native-tests are running successfully with this fix.

@amitkdutta amitkdutta requested a review from a team as a code owner November 23, 2024 05:57
@amitkdutta
Copy link
Contributor Author

@tdcmeehan @aditi-pandit Yes, will delete CheckUnsupportedPrestissimoTypes. We just launched timestamp and ipaddress support a week ago, and as the features are new, it could be possible to hit issues in next few weeks. If anything observed, we will have the opportunity to disable timestamp but keep the build with other fixes as each release contains many new correctness fixes with velox. Henece, I want to keep the check alive but disable by default. Once we pass through a few releases, we will be more confident to delete it completely.

@amitkdutta amitkdutta force-pushed the enable_1 branch 5 times, most recently from bc36e26 to 54a4fac Compare November 23, 2024 20:13
aditi-pandit
aditi-pandit approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amitkdutta

@amitkdutta amitkdutta merged commit b823353 into prestodb:master Nov 25, 2024
60 checks passed
@amitkdutta
Copy link
Contributor Author

Now we have enabled all the timestamp and ipaddress tests in CI, and found one result mismatch issue with checksum and ipaddress

Disabled the test for now and created an issue
#24130
CC: @kgpai @pramodsatya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pramodsatya pramodsatya pramodsatya left review comments

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@feilong-liu feilong-liu feilong-liu approved these changes

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

@ClarenceThreepwood ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees
No one assigned
Labels
from:Meta PR from Meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@amitkdutta @tdcmeehan @aditi-pandit @feilong-liu @pramodsatya @prestodb-ci